🐛 Remove cluster label - #142

pkg/server/grpc/metrics/metrics_test.go (2)

143-145: Byte-counter expectations may be brittle across protobuf changes.

Asserting exact values (2286, 69) can become flaky with minor proto or marshalling changes. Consider asserting presence and “> 0” (or a small range) instead of exact equality to reduce test fragility.

---

114-114: Typo: “gaugge” → “gauge”.

Minor comment nit.

- // assert gaugge and counter metrics
+ // assert gauge and counter metrics

pkg/cloudevents/generic/metrics_collector.go (3)

224-234: Fix comment: “received” vs “sent”.

The docstring above increaseCloudEventsReceivedCounter says “sent” but the function increments the received counter.

-// increaseCloudEventsReceivedCounter increases the cloudevents sent counter metric:
+// increaseCloudEventsReceivedCounter increases the cloudevents received counter metric:

---

104-109: Docs nit: “second” → “seconds”.

Metric names are *_duration_seconds; comments should match.

-// The resource spec resync duration metric is a histogram with a base metric name of 'resource_spec_resync_duration_second'
+// The resource spec resync duration metric is a histogram with a base metric name of 'resource_spec_resync_duration_seconds'
-// The resource status resync duration metric is a histogram with a base metric name of 'resource_status_resync_duration_second'
+// The resource status resync duration metric is a histogram with a base metric name of 'resource_status_resync_duration_seconds'

Also applies to: 138-145

---

225-233: Unused cluster params in helpers (intentional) — consider follow-up cleanup.

These functions still accept cluster but no longer use it for labels. If these are package-internal, consider dropping or renaming the param to “_ string” to signal intentional non-use and avoid confusion. Defer if API stability is a concern for this PR.

Example (one function shown):

-func increaseCloudEventsReceivedCounter(source, cluster, dataType, subresource, action string) {
+func increaseCloudEventsReceivedCounter(source, _ string, dataType, subresource, action string) {

Also applies to: 236-248, 251-258, 261-268

pkg/cloudevents/generic/metrics_collector_test.go (1)

98-100: Reduce time-based flakiness by polling instead of fixed sleeps.

Replace fixed sleeps with polling (require.Eventually or a simple loop with timeout) to make tests resilient under load/CI variance.

Example sketch:

require.Eventually(t, func() bool {
  // read counter and compare against expected
  return int(toFloat64Counter(receivedTotal)) == want
}, 2*time.Second, 50*time.Millisecond)

Also applies to: 241-243, 281-282

pkg/cloudevents/server/grpc/metrics/metrics.go (3)

108-131: Consider counting “called” and “msg_received” even on early unary errors.

Currently, called_total and msg_received_total are incremented only after successful CE parse; early errors (bad request type, CE conversion/type parse failures) record processed/duration but not called/received. If you want “called” to reflect every RPC invocation (successful or not), increment those counters with data_type="unknown" before the early returns.

- pubReq, ok := req.(*pbv1.PublishRequest)
+ pubReq, ok := req.(*pbv1.PublishRequest)
   if !ok {
-    err := fmt.Errorf("invalid request type for Publish method")
-    recordCloudEventsMetrics(dataType, method, err, startTime)
+    err := fmt.Errorf("invalid request type for Publish method")
+    grpcCECalledCountMetric.WithLabelValues(dataType, method).Inc()
+    grpcCEMessageReceivedCountMetric.WithLabelValues(dataType, method).Inc()
+    recordCloudEventsMetrics(dataType, method, err, startTime)
     return nil, err
   }
   // convert the request to cloudevent and extract the source
   evt, err := binding.ToEvent(ctx, protocol.NewMessage(pubReq.Event))
   if err != nil {
-    err = fmt.Errorf("failed to convert to cloudevent: %v", err)
-    recordCloudEventsMetrics(dataType, method, err, startTime)
+    err = fmt.Errorf("failed to convert to cloudevent: %v", err)
+    grpcCECalledCountMetric.WithLabelValues(dataType, method).Inc()
+    grpcCEMessageReceivedCountMetric.WithLabelValues(dataType, method).Inc()
+    recordCloudEventsMetrics(dataType, method, err, startTime)
     return nil, err
   }
   // extract the data type from event type
   eventType, err := types.ParseCloudEventsType(evt.Type())
   if err != nil {
-    err = fmt.Errorf("failed to parse cloud event type %s, %v", evt.Type(), err)
-    recordCloudEventsMetrics(dataType, method, err, startTime)
+    err = fmt.Errorf("failed to parse cloud event type %s, %v", evt.Type(), err)
+    grpcCECalledCountMetric.WithLabelValues(dataType, method).Inc()
+    grpcCEMessageReceivedCountMetric.WithLabelValues(dataType, method).Inc()
+    recordCloudEventsMetrics(dataType, method, err, startTime)
     return nil, err
   }
- dataType = eventType.CloudEventsDataType.String()
+ dataType = eventType.CloudEventsDataType.String()
+ grpcCECalledCountMetric.WithLabelValues(dataType, method).Inc()
+ grpcCEMessageReceivedCountMetric.WithLabelValues(dataType, method).Inc()
- grpcCECalledCountMetric.WithLabelValues(dataType, method).Inc()
- grpcCEMessageReceivedCountMetric.WithLabelValues(dataType, method).Inc()

If prior behavior was intentional, please confirm and disregard.

Also applies to: 134-146

---

169-175: Update comment: no longer capturing cluster in stream path.

Comment still mentions “captures the cluster and data type”; cluster is removed.

-// It captures the cluster and data type from the SubscriptionRequest and emits metrics.
+// It captures the data type from the SubscriptionRequest and emits metrics.

---

96-107: Optional: guard by method == "Publish" in unary interceptor.

If additional unary methods are ever added to the CloudEventService, the current logic will treat them as Publish and error on type assertions. Consider short-circuiting when method != "Publish".

- if service != gRPCCloudEventService {
+ if service != gRPCCloudEventService {
     return handler(ctx, req)
   }
+ if _, method := SplitMethod(info.FullMethod); method != "Publish" {
+   return handler(ctx, req)
+ }

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: morvencao
PR: open-cluster-management-io/sdk-go#138
File: pkg/server/grpc/metrics/metrics.go:230-236
Timestamp: 2025-08-27T03:21:36.789Z
Learning: In the gRPC metrics implementation for open-cluster-management.io/sdk-go, the grpc_server_ce_called_total metric intentionally has different semantics for unary vs stream calls: unary calls increment it once per RPC invocation, while stream calls increment it once per received message. This is deliberate design to provide granular visibility into stream message activity.

Learnt from: morvencao
PR: open-cluster-management-io/sdk-go#138
File: pkg/cloudevents/server/grpc/metrics/metrics.go:231-254
Timestamp: 2025-09-01T03:34:05.141Z
Learning: In open-cluster-management.io/sdk-go gRPC CloudEvents metrics, processing duration metrics should only be recorded for unary RPCs, not stream RPCs. Stream RPCs can be long-lived connections that persist as long as the gRPC server runs, making duration metrics confusing and less useful for operators debugging issues.

pkg/cloudevents/generic/metrics_collector.go (2)

31-38: Label sets updated to use consumer — OK, but watch cardinality

The label swaps look correct across received/sent/resync metrics. Please ensure consumer values are bounded and stable (avoid per-connection IDs) to keep Prometheus cardinality under control. Consider adding a brief doc comment noting that any cluster mapping should be handled via scrape-time relabeling, per PR intent.

Also applies to: 40-48, 50-56

---

108-113: Docs: “second” → “seconds” (nit) and metric name wording

Minor: the prose says “…_duration_second” but the metric name is “…_duration_seconds”. Recommend fixing the wording to match.

Apply this diff to the comment text only:

-// The resource spec resync duration metric is a histogram with a base metric name of 'resource_spec_resync_duration_second'
+// The resource spec resync duration metric is a histogram with a base metric name of 'resource_spec_resync_duration_seconds'
...
-// The resource status resync duration metric is a histogram with a base metric name of 'resource_status_resync_duration_second'
+// The resource status resync duration metric is a histogram with a base metric name of 'resource_status_resync_duration_seconds'

Also applies to: 142-147

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

pkg/cloudevents/generic/metrics_collector.go (1)

96-99: Docs: sent_total examples omit consumer label but code requires it

Examples should reflect the declared label set; otherwise they’ll mislead operators.

Apply this doc-only tweak:

-// cloudevents_sent_total{source="source1",original_source="none",type="io.open-cluster-management.works.v1alpha1.manifestbundles",subresource="spec",action="create"} 1
-// cloudevents_sent_total{source="consumer1-work-agent",original_source="source1",type="io.open-cluster-management.works.v1alpha1.manifestbundles",subresource="status",action="update"} 2
+// cloudevents_sent_total{source="source1",original_source="none",consumer="consumer1",type="io.open-cluster-management.works.v1alpha1.manifestbundles",subresource="spec",action="create"} 1
+// cloudevents_sent_total{source="consumer1-work-agent",original_source="source1",consumer="",type="io.open-cluster-management.works.v1alpha1.manifestbundles",subresource="status",action="update"} 2

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: morvencao
PR: open-cluster-management-io/sdk-go#138
File: pkg/cloudevents/server/grpc/metrics/metrics.go:231-254
Timestamp: 2025-09-01T03:34:05.141Z
Learning: In open-cluster-management.io/sdk-go gRPC CloudEvents metrics, processing duration metrics should only be recorded for unary RPCs, not stream RPCs. Stream RPCs can be long-lived connections that persist as long as the gRPC server runs, making duration metrics confusing and less useful for operators debugging issues.

pkg/cloudevents/generic/metrics_collector.go (1)

279-289: Good fix: Stable label cardinality on counters

Consumer is now always present in labels (even if empty) and original_source defaults to "none", preventing With() panics.

Also applies to: 302-316

pkg/cloudevents/generic/metrics_collector.go (1)

67-73: Verify: Agent status-resync histogram still carries consumer label

The shared resync histogram label set includes consumer, and updateResourceStatusResyncDurationMetric() is called by the agent (see agentclient.go Line 168). If the objective is “no cluster/consumer label from agent,” this contradicts it. Confirm whether:

• agent-side resync duration should omit consumer entirely (new Vec without consumer), or
• keeping consumer on histograms is acceptable and only counters drop it.

If needed, I can propose a split of label sets/metrics for source vs agent resync durations.

Also applies to: 345-353

pkg/cloudevents/generic/sourceclient.go (2)

132-134: Avoid potential panic on clusterName extraction

Use safe conversion to prevent a type-assertion panic if the extension is missing or not a string.

- clusterName := evt.Context.GetExtensions()[types.ExtensionClusterName].(string)
+ clusterName, _ := cloudeventstypes.ToString(
+   evt.Context.GetExtensions()[types.ExtensionClusterName],
+ )

---

244-248: Minor grammar nit in log message

- klog.V(4).Infof("there are is no objs from the list, do nothing")
+ klog.V(4).Infof("no objects returned from the list; nothing to do")

pkg/cloudevents/generic/agentclient.go (1)

168-169: Confirm agent histogram labeling vs PR goal

updateResourceStatusResyncDurationMetric(evt.Source(), c.clusterName, ...) will emit consumer=clusterName from the agent. If agent metrics must drop cluster/consumer entirely, consider a consumer-less histogram for the agent path or relabel-drop in the scrape pipeline.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: morvencao
PR: open-cluster-management-io/sdk-go#138
File: pkg/cloudevents/server/grpc/metrics/metrics.go:231-254
Timestamp: 2025-09-01T03:34:05.141Z
Learning: In open-cluster-management.io/sdk-go gRPC CloudEvents metrics, processing duration metrics should only be recorded for unary RPCs, not stream RPCs. Stream RPCs can be long-lived connections that persist as long as the gRPC server runs, making duration metrics confusing and less useful for operators debugging issues.

pkg/cloudevents/generic/metrics_collector.go (2)

67-73: Confirm intent: resync duration metrics still require the consumer label

PR objective says agent-produced metrics should remove the cluster label; here, resync histograms always include consumer. If agents emit these, you'll retain a cluster-derived label in practice. If the intent is “no consumer on agent,” consider splitting resync histograms into agent/server variants (agent: source,type; server: source,consumer,type). Otherwise, pass consumer="" from agent call sites to minimize cardinality.

I can draft a minimal split (new ByAgent/BySource histograms and wrappers) if you confirm the desired surface.

---

151-156: NIT: comments say “second” but metric is “seconds”

Minor doc accuracy fix.

-// The resource spec resync duration metric is a histogram with a base metric name of 'resource_spec_resync_duration_second'
+// The resource spec resync duration metric is a histogram with a base metric name of 'resource_spec_resync_duration_seconds'
...
-// The resource status resync duration metric is a histogram with a base metric name of 'resource_status_resync_duration_second'
+// The resource status resync duration metric is a histogram with a base metric name of 'resource_status_resync_duration_seconds'

Also applies to: 185-191

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: morvencao
PR: open-cluster-management-io/sdk-go#138
File: pkg/cloudevents/server/grpc/metrics/metrics.go:231-254
Timestamp: 2025-09-01T03:34:05.141Z
Learning: In open-cluster-management.io/sdk-go gRPC CloudEvents metrics, processing duration metrics should only be recorded for unary RPCs, not stream RPCs. Stream RPCs can be long-lived connections that persist as long as the gRPC server runs, making duration metrics confusing and less useful for operators debugging issues.

pkg/cloudevents/generic/metrics_collector.go (2)

257-267: Cardinality fix looks good

Always including consumer in source-side counters resolves the inconsistent-label panic raised earlier.

Also applies to: 280-294

---

95-108: Make CloudEvents metric names unique to avoid Prometheus registration panics

All four CounterVecs use the same Name constants (receivedCounterMetric="received_total", sentCounterMetric="sent_total") in pkg/cloudevents/generic/metrics_collector.go; this will panic if both RegisterClientCloudEventsMetrics and RegisterSourceCloudEventsMetrics run in one process. Give each CounterVec a unique Name and update the doc examples.

Apply:

-    Name:      receivedCounterMetric,
+    Name:      "received_by_source_total",

-    Name:      receivedCounterMetric,
+    Name:      "received_by_client_total",

-    Name:      sentCounterMetric,
+    Name:      "sent_from_source_total",

-    Name:      sentCounterMetric,
+    Name:      "sent_from_client_total",

Update doc examples accordingly:

-// cloudevents_received_total{source="agent1",consumer="consumer1",type="io.open-cluster-management.works.v1alpha1.manifests",subresource="spec",action="create"} 1
-// cloudevents_received_total{source="agent1",consumer="consumer1",type="io.open-cluster-management.works.v1alpha1.manifests",subresource="spec",action="update"} 1
+// cloudevents_received_by_source_total{source="agent1",consumer="consumer1",type="io.open-cluster-management.works.v1alpha1.manifests",subresource="spec",action="create"} 1
+// cloudevents_received_by_source_total{source="agent1",consumer="consumer1",type="io.open-cluster-management.works.v1alpha1.manifests",subresource="spec",action="update"} 1

-// cloudevents_received_total{source="source1",type=...,subresource="spec",action="create"} 1
-// cloudevents_received_total{source="source1",type=...,subresource="spec",action="update"} 1
+// cloudevents_received_by_client_total{source="source1",type=...,subresource="spec",action="create"} 1
+// cloudevents_received_by_client_total{source="source1",type=...,subresource="spec",action="update"} 1

-// cloudevents_sent_total{source="source1",original_source="none",consumer="consumer1",type=...,subresource="spec",action="create"} 1
+// cloudevents_sent_from_source_total{source="source1",original_source="none",consumer="consumer1",type=...,subresource="spec",action="create"} 1

-// cloudevents_sent_total{source="consumer1-work-agent",original_source="source1",type=...,subresource="status",action="update"} 2
+// cloudevents_sent_from_client_total{source="consumer1-work-agent",original_source="source1",type=...,subresource="status",action="update"} 2

pkg/cloudevents/generic/metrics_collector.go (1)

110-116: Unify terminology: rename 'Agent' → 'Client' for helper funcs and comments

Rename the two unexported helpers and update their callers.

• pkg/cloudevents/generic/metrics_collector.go: rename increaseCloudEventsReceivedByAgentCounter (def at line 270) → increaseCloudEventsReceivedByClientCounter; change comment "received by agent" → "received by client".
• pkg/cloudevents/generic/metrics_collector.go: rename increaseCloudEventsSentFromAgentCounter (def at line 297) → increaseCloudEventsSentFromClientCounter; change comment "sent from agent" → "sent from client".
• Update callers in pkg/cloudevents/generic/agentclient.go (lines 114, 135, 156).

-// The cloudevents received by agent counter metric...
+// The cloudevents received by client counter metric...
-func increaseCloudEventsReceivedByAgentCounter(...) {
+func increaseCloudEventsReceivedByClientCounter(...) {

-// The cloudevents sent from agent counter metric...
+// The cloudevents sent from client counter metric...
-func increaseCloudEventsSentFromAgentCounter(...) {
+func increaseCloudEventsSentFromClientCounter(...) {

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: morvencao
PR: open-cluster-management-io/sdk-go#138
File: pkg/cloudevents/server/grpc/metrics/metrics.go:231-254
Timestamp: 2025-09-01T03:34:05.141Z
Learning: In open-cluster-management.io/sdk-go gRPC CloudEvents metrics, processing duration metrics should only be recorded for unary RPCs, not stream RPCs. Stream RPCs can be long-lived connections that persist as long as the gRPC server runs, making duration metrics confusing and less useful for operators debugging issues.

Learnt from: morvencao
PR: open-cluster-management-io/sdk-go#138
File: pkg/cloudevents/server/grpc/metrics/metrics.go:231-254
Timestamp: 2025-09-01T03:34:05.141Z
Learning: In open-cluster-management.io/sdk-go gRPC CloudEvents metrics, processing duration metrics should only be recorded for unary RPCs, not stream RPCs. Stream RPCs can be long-lived connections that persist as long as the gRPC server runs, making duration metrics confusing and less useful for operators debugging issues.

pkg/cloudevents/generic/metrics_collector.go (6)

241-255: Register reconnect metric in client registrar, remove from source registrar

This was previously suggested and remains unaddressed.

 func RegisterClientCloudEventsMetrics(register prometheus.Registerer) {
 	register.MustRegister(cloudeventsReceivedByClientCounterMetric)
 	register.MustRegister(cloudeventsSentFromClientCounterMetric)
 	register.MustRegister(resourceStatusResyncDurationMetric)
+	register.MustRegister(clientReconnectedCounterMetric)
 	register.MustRegister(workProcessedCounterMetric)
 }
 
 func RegisterSourceCloudEventsMetrics(register prometheus.Registerer) {
 	register.MustRegister(cloudeventsReceivedBySourceCounterMetric)
 	register.MustRegister(cloudeventsSentFromSourceCounterMetric)
 	register.MustRegister(resourceSpecResyncDurationMetric)
-	register.MustRegister(clientReconnectedCounterMetric)
 }

---

116-123: Same: unique name for client 'received' counter

 var cloudeventsReceivedByClientCounterMetric = prometheus.NewCounterVec(
   prometheus.CounterOpts{
     Subsystem: cloudeventsMetricsSubsystem,
-    Name:      receivedCounterMetric,
+    Name:      "received_by_client_total",
     Help:      "The total number of CloudEvents received by client.",
   },
   cloudeventsReceivedByClientMetricsLabels,
 )

---

125-136: Same: unique name for source 'sent' counter

 var cloudeventsSentFromSourceCounterMetric = prometheus.NewCounterVec(
   prometheus.CounterOpts{
     Subsystem: cloudeventsMetricsSubsystem,
-    Name:      sentCounterMetric,
+    Name:      "sent_from_source_total",
     Help:      "The total number of CloudEvents sent from source.",
   },
   cloudeventsSentFromSourceMetricsLabels,
 )

---

138-149: Same: unique name for client 'sent' counter

 var cloudeventsSentFromClientCounterMetric = prometheus.NewCounterVec(
   prometheus.CounterOpts{
     Subsystem: cloudeventsMetricsSubsystem,
-    Name:      sentCounterMetric,
+    Name:      "sent_from_client_total",
     Help:      "The total number of CloudEvents sent from client.",
   },
   cloudeventsSentFromClientMetricsLabels,
 )

---

95-109: Make metric names unique to avoid registration panic and align docs

Both received counters share Name="received_total". Use distinct names.

 var cloudeventsReceivedBySourceCounterMetric = prometheus.NewCounterVec(
   prometheus.CounterOpts{
     Subsystem: cloudeventsMetricsSubsystem,
-    Name:      receivedCounterMetric,
+    Name:      "received_by_source_total",
     Help:      "The total number of CloudEvents received by source.",
   },
   cloudeventsReceivedBySourceMetricsLabels,
 )

---

67-73: Split resync label sets: spec includes consumer; status does not

Current shared cloudeventsResyncMetricsLabels includes consumer, which conflicts with agent-side (status) metrics. Define separate label arrays and use them for the respective histograms.

-// cloudeventsResyncMetricsLabels - Array of labels added to cloudevents resync metrics:
-var cloudeventsResyncMetricsLabels = []string{
-	metricsSourceLabel,   // source
-	metricsConsumerLabel, // consumer
-	metricsDataTypeLabel, // data type, e.g. manifests, manifestbundles
-}
+// resourceSpecResyncMetricsLabels: source-side (includes consumer)
+var resourceSpecResyncMetricsLabels = []string{
+	metricsSourceLabel,   // source
+	metricsConsumerLabel, // consumer
+	metricsDataTypeLabel, // type
+}
+
+// resourceStatusResyncMetricsLabels: client-side (no consumer)
+var resourceStatusResyncMetricsLabels = []string{
+	metricsSourceLabel,   // source
+	metricsDataTypeLabel, // type
+}

pkg/cloudevents/generic/agentclient.go (1)

149-157: Optional: align function naming (Agent vs Client)

increaseCloudEventsReceivedByAgentCounter is incrementing the client counter. Consider renaming to increaseCloudEventsReceivedByClientCounter for consistency with metric vars/tests.

pkg/cloudevents/generic/metrics_collector.go (2)

191-201: Fix status resync histogram docs: remove consumer label in examples

Agent-side status resync must not show consumer/cluster.

-// resource_status_resync_duration_seconds_bucket{source="source1",consumer="consumer1",type="io.open-...manifests",le="0.5"} 1
+// resource_status_resync_duration_seconds_bucket{source="source1",type="io.open-...manifests",le="0.5"} 1
-// resource_status_resync_duration_seconds_sum{source="source1",consumer="consumer1",type="io.open-...manifests"} 1.6
+// resource_status_resync_duration_seconds_sum{source="source1",type="io.open-...manifests"} 1.6
-// resource_status_resync_duration_seconds_count{source="source1",consumer="consumer1",type="io.open-...manifests"} 2
+// resource_status_resync_duration_seconds_count{source="source1",type="io.open-...manifests"} 2

---

95-106: Docs vs code mismatch: fix comment header

Comment mentions "received_by_source_total" but Name was "received_total". The Name fix above will resolve this inconsistency.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: morvencao
PR: open-cluster-management-io/sdk-go#138
File: pkg/cloudevents/server/grpc/metrics/metrics.go:231-254
Timestamp: 2025-09-01T03:34:05.141Z
Learning: In open-cluster-management.io/sdk-go gRPC CloudEvents metrics, processing duration metrics should only be recorded for unary RPCs, not stream RPCs. Stream RPCs can be long-lived connections that persist as long as the gRPC server runs, making duration metrics confusing and less useful for operators debugging issues.

Learnt from: morvencao
PR: open-cluster-management-io/sdk-go#138
File: pkg/cloudevents/server/grpc/metrics/metrics.go:231-254
Timestamp: 2025-09-01T03:34:05.141Z
Learning: In open-cluster-management.io/sdk-go gRPC CloudEvents metrics, processing duration metrics should only be recorded for unary RPCs, not stream RPCs. Stream RPCs can be long-lived connections that persist as long as the gRPC server runs, making duration metrics confusing and less useful for operators debugging issues.

pkg/cloudevents/generic/metrics_collector_test.go (2)

115-116: Reset the client registry for reconnect test

Reconnect is a client-side metric; reset the client registry to avoid stale samples.

Apply:

-	ResetSourceCloudEventsMetrics()
+	ResetClientCloudEventsMetrics()

Verify the reset location of the reconnect metric:

#!/bin/bash
# Expectation: clientReconnectedCounterMetric.Reset() should live in ResetClientCloudEventsMetrics().
rg -nC2 'func Reset(Source|Client)CloudEventsMetrics' pkg/cloudevents/generic/metrics_collector.go
rg -n 'clientReconnectedCounterMetric\.Reset\(' pkg/cloudevents/generic/metrics_collector.go

---

301-303: Client received counter label set fixed—LGTM

Matches the new taxonomy (no cluster label on client-side metrics).

pkg/cloudevents/generic/metrics_collector_test.go (2)

99-101: Replace fixed sleep with an eventually check to de-flake

Polling the counters is more robust than a 1s sleep on slower CI.

Apply:

-			// wait 1 second for agent receive the resources
-			time.Sleep(time.Second)
+			// wait until metrics reflect the published resources (avoid flakiness)
+			require.Eventually(t, func() bool {
+				sent := cloudeventsSentFromSourceCounterMetric.WithLabelValues(
+					c.sourceID, noneOriginalSource, c.clusterName, c.dataType.String(), string(c.subresource), string(c.action),
+				)
+				recv := cloudeventsReceivedByClientCounterMetric.WithLabelValues(
+					c.sourceID, c.dataType.String(), string(c.subresource), string(c.action),
+				)
+				return int(toFloat64Counter(sent)) == len(c.resources) &&
+					int(toFloat64Counter(recv)) == len(c.resources)
+			}, 5*time.Second, 10*time.Millisecond)

---

236-241: Relax the upper bound on resync duration to avoid time-based flakes

The 1s ceiling is brittle under loaded CI; keep >0 and use a looser upper bound.

Apply:

-				require.Greater(t, sum, 0.0)
-				require.Less(t, sum, 1.0)
+				require.Greater(t, sum, 0.0)
+				require.Less(t, sum, 5.0)

-				require.Greater(t, sum, 0.0)
-				require.Less(t, sum, 1.0)
+				require.Greater(t, sum, 0.0)
+				require.Less(t, sum, 5.0)

Also applies to: 308-313

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: morvencao
PR: open-cluster-management-io/sdk-go#138
File: pkg/cloudevents/server/grpc/metrics/metrics.go:231-254
Timestamp: 2025-09-01T03:34:05.141Z
Learning: In open-cluster-management.io/sdk-go gRPC CloudEvents metrics, processing duration metrics should only be recorded for unary RPCs, not stream RPCs. Stream RPCs can be long-lived connections that persist as long as the gRPC server runs, making duration metrics confusing and less useful for operators debugging issues.